fix: sanitise GITHUB_REF to be a valid tag value #259
Conversation
dgholz
changed the title
|
@Mergifyio refresh |
|
Command
|
|
@dgholz Thanks for your contribution, could you please attach your test results/screenshots of showing how your branch name was outside of the |
I've added a test that verifies the AssumeRole call is made with a sanitized branch name, where the branch name from the environment had a lot of characters that were not suitable for a tag value: https://github.com/aws-actions/configure-aws-credentials/pull/259/files#diff-59e25b254be93038f106111be580b6fb54c6963b6c4e7ef744e58fb8f2b3e3a2R618. If you approve the pending run in https://github.com/aws-actions/configure-aws-credentials/actions/runs/1233568010 you'll see the test result.
Ran our workflow that uses this action with a branch with a |
|
@paragbhingre hi, is there anything else I can do to make this easy to merge? Happy to share a workflow that now works, screenshots, etc. |
|
@paragbhingre hello again, is there anything I can do to help? |
|
@paragbhingre hello, what can I do to make it easy to review this? |
|
@paragbhingre hello again, I've rebased onto latest master and I see the checks waiting for a maintainer to approve them for running in https://github.com/aws-actions/configure-aws-credentials/actions/runs/1653379542. Could you please approve it?
Just to be thorough: the branch that originally caused me to make this PR was something like |
|
@paragbhingre hi, anything you'd like me to do to make reviewing this PR a piece of cake? |
|
@paragbhingre hello, is there someone else available to review this PR? Should I ask them? |
|
@dgholz so sorry for the late reply, I am no longer working on github actions but I will surely find someone to look at this or find some time to take a look at it. apologies again. |
There was a problem hiding this comment.
Thanks for your submission and patience @dgholz, however I'm having trouble reproducing this issue to begin with.
Ran our workflow that uses this action with a branch with a # in its name.
This isn't enough for me to reproduce the issue, I'm running this action fine on a branch called #@#%&001-Branch (and many more attempts with various branch names).
It makes sense to me why this issue is occurring, the character requirements are specified here. However I am not sure why I cannot reproduce. This works for me:
jobs:
deploy:
name: Upload to Amazon S3
runs-on: ubuntu-latest
# These permissions are needed to interact with GitHub's OIDC Token endpoint.
permissions:
id-token: write
contents: read
steps:
- name: Checkout
uses: actions/checkout@v2
- name: Configure AWS credentials from Test account
uses: aws-actions/configure-aws-credentials@v1
with:
role-to-assume: arn:aws:iam::123456789012:role/MyRole
role-duration-seconds: 6000
aws-region: us-east-1
aws-access-key-id: xxx
aws-secret-access-key: xxx
Your PR looks great and I'd love to approve it, however I do need to confirm the current behavior before I approve this. Is there any advice you can give to help reproduce this issue?
|
Since I'm not able to reproduce this, in other words: This means that either there are a stricter set of conditions where this requirement is enforced (meaning this would be a breaking change), or that the restriction with these characters has been lifted in the time since this issue and PR has been active and the documentation doesn't reflect that. We need more investigation into the root cause of this issue, and a consistent way to reproduce it before we can fix this issue. Going to close this PR and request that discussion continue on the original issue, thanks! 🙂 |
|
hi @peterwoodworth , thanks for finding time to take a look at this. Try reproducing without using the OIDC token and instead with explicit configure-aws-credentials/index.js Line 92 in 5820660 |
Fixes #247
Description of changes:
Applies the same sanitization to Branch as is applied to Workflow.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.